Run scheduled maintenance securely on every site#1862
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces request-triggered pruning with durable scheduled maintenance, adds authenticated ChangesScheduled maintenance platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
stefan-burke
left a comment
stefan-burke
left a comment
There was a problem hiding this comment.
Submitting the pending review so the verified fixes can be replied to and resolved individually.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Outside-diff finding: runner budget reserve. Resolved in 475535d. The literal is now TASK_RUNNER_CALL_RESERVE with a comment naming the claim, finish, and final no-work claim calls it protects. Runner tests preserve the existing capacity behavior. |
|
Outside-diff finding: revalidate invite state at deletion. Resolved in 475535d. Every child and user DELETE now selects only ids that are still unactivated in the same batch, preventing an accepted invite from being removed after candidate discovery. The invite pruning tests cover the state-change race. |
|
Outside-diff finding: bounded encrypted-invite scan. Resolved in 475535d. Invite pruning now reads at most MAINTENANCE_PRUNE_BATCH + 1 ordered candidates per run, persists the last id as its checkpoint, and decrypts only the bounded page. The large-current-prefix regression test proves forward progress without an unbounded scan. |
|
Outside-diff finding: built-sites module size. Resolved in 475535d. Blob schema/validation, field mapping, and types moved into src/shared/db/built-sites/blob.ts, fields.ts, and types.ts; the I/O module is now 318 lines. Direct tests cover each extracted module. |
|
Outside-diff finding: built-site panel hook. Verified and no change is needed. The beforeAll is inside describe("built site maintenance panel"), so it is suite-scoped rather than a module-level global BDD hook; it does not make the file ungroupable under the repository rule. Direct component coverage was strengthened in 5dabd8d. |
|
Outside-diff finding: scheduled-key validation before persistence. Resolved in 475535d. buildSiteDataBlobFromInput parses the v2 blob with the isScheduledTaskKey check before serialization reaches the table insert/update. The blob regression tests reject invalid keys and cover valid/null values. |
|
Outside-diff finding: equivalent-mutants merge conflict. Resolved by the branch merges through 0613331. The file contains the entries from both histories with no conflict markers; the final precommit, including lint, typecheck, duplicate detection, build, and full coverage suite, passed before 7d90730 was committed. |
stefan-burke
left a comment
stefan-burke
left a comment
There was a problem hiding this comment.
Submitting resolved review-thread replies.
stefan-burke
left a comment
stefan-burke
left a comment
There was a problem hiding this comment.
Submitting resolved review-thread replies.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e9fdef3e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
stefan-burke
left a comment
stefan-burke
left a comment
There was a problem hiding this comment.
Submitting the final resolved review-thread replies.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d28fb6e928
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| MAINTENANCE_REQUEST_CALL_LIMIT - used, | ||
| ), | ||
| ); | ||
| if (combinedAllowance === 0) return Promise.resolve(); |
There was a problem hiding this comment.
When an organic GET has one subrequest left under the 40-call maintenance envelope, this zero-only guard still enters the runner. On a normal completed site, the registry checks activity_log_backfill.enabled with a DB read and then syncMaintenanceTaskRows performs a batched write before the loop can decide no task fits, so the second bookkeeping call exceeds the one-call allowance and reports a false organic-maintenance failure for an otherwise successful foreground request. Skip unless the remaining allowance can cover the runner's startup bookkeeping, not only when it is exactly zero.
Useful? React with 👍 / 👎.
| if (!deployResult.ok) return deployResult; | ||
| return okResult({ | ||
| defaultHostname: deployResult.value, | ||
| defaultHostname: `https://${createResult.value.slug}.deno.dev`, |
There was a problem hiding this comment.
For Deno-hosted builds this now stores https://<app>.deno.dev, but that is the old Deploy Classic shape; the current Deno Deploy docs describe v2 production domains as <app>.<organization>.deno.net (for example, my-app.acme-inc.deno.net). Any Deno child retained through this path gets an unreachable siteUrl, so assignment emails, renewal links, and scheduled-key verification target the wrong host even after the app publishes successfully.
Useful? React with 👍 / 👎.
| return errorResult("Deploy code failed: no hostname in response"); | ||
| } | ||
| return okResult(`https://${hostname}`); | ||
| return okResult(undefined); |
There was a problem hiding this comment.
With the request now going to the documented /deploy endpoint, Deno's v2 OpenAPI says this call returns 202 with a revision whose status can still be queued, building, or failed; this line treats that acceptance as a finished publish. For Deno-hosted builds and updates, a later build failure leaves the built-site row retained or the update flash successful even though no working revision was routed. Poll the revision/progress to a terminal succeeded state before returning success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 079d3b32f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const SiteDataBlobSchema = v.variant("v", [ | ||
| v.strictObject({ ...siteDataFields, v: v.literal(1) }), |
There was a problem hiding this comment.
For built-site rows written before the blob version field existed, the previous schema accepted a missing v and defaulted it to version 1; this discriminated v.variant("v", ...) now requires the v field to be present. Any operator with one of those legacy built_sites.site_data blobs will get an "Invalid stored JSON" failure when built sites are loaded, which breaks the built-sites pages and assignment paths until the row is manually rewritten. Keep the v1 arm accepting a missing discriminator, or migrate those blobs before switching to a required variant.
Useful? React with 👍 / 👎.
| createClient: (url: string, authToken: string): Client => | ||
| createClient(authToken ? { authToken, url } : { url }), | ||
| createClient({ authToken, url }), |
There was a problem hiding this comment.
When a built-site page reads a child's version, this factory returns a raw libsql client, so the later client.execute in withSiteDb does not pass through countDatabaseRoundTrip like the main DB client. On Bunny-hosted child databases those libsql requests still consume subrequests, so a request near the maintenance or 50-call limit can be undercounted and then run organic maintenance or fail at the platform before our guard fires. Wrap this client with the same round-trip guard, or count each child DB execute.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14e303ff0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| url: string, | ||
| init?: RequestInit, | ||
| ): Promise<FetchResult> => { | ||
| countExternalSubrequest(`fetch ${new URL(url).origin}`); |
There was a problem hiding this comment.
This only adds subrequest accounting to fetchText, but Stripe checkout/session/refund calls do not use that helper: src/shared/stripe/request.ts sends each retry attempt through config.fetch(...) directly. On a Stripe booking, webhook, balance payment, or refund request, those external provider calls are therefore invisible to getSubrequestUsage(), so a request near the 40/50-call envelope can still run organic maintenance or pass the guard and then hit Bunny's subrequest cap. Count inside the Stripe transport attempt as well as in fetchText.
Useful? React with 👍 / 👎.
| }, | ||
| { | ||
| deadlineMs: 10_000, | ||
| enabled: async () => Boolean(settings.publicKey) && hasLegacyActivityLog(), |
There was a problem hiding this comment.
After a site has already converted all legacy activity-log rows, this predicate still runs hasLegacyActivityLog() on every organic or scheduled maintenance wake. That query filters activity_log.message LIKE 'enc:%', but the schema only indexes attendee_id and listing_id, so a site with a large all-converted log does a full activity-log scan every minute of organic traffic just to decide the task is disabled. Keep a durable done marker/checkpoint, or mark the task complete after a zero-row batch, so completed sites stop probing the unindexed log.
Useful? React with 👍 / 👎.
1 participant
What changed
POST /scheduledwith a unique key for each site before setup, routing, or database work.Key rotation
Coordinated one-click key rotation is intentionally deferred to the upcoming Uptime Kuma integration. That work will update the child site and its monitor together. Until then, a host operator can manually replace a compromised key on the child and in the monitor.
This PR does not include a second key slot, staged keys, promotion actions, or provider-specific rotation APIs.
Why
Quiet sites still need cleanup, but the old public endpoint could trigger database work and builder fan-out. Each site now has a small, bounded maintenance endpoint that a monitor can call safely.
Checks
mainis merged through9e9fdef3; final review fixes are pushed throughd28fb6e9.